[Kernel][Metrics][PR#2] Add SnapshotReport for reporting snapshot construction #3903

allisonport-db · 2024-11-26T19:43:17Z

Which Delta project/connector is this regarding?

Description

Adds a SnapshotReport for reporting snapshot construction.

We record a SnapshotReport after successfully constructing a snapshot or if an exception is thrown during construction. We use SnapshotContext to propagate and update information about the snapshot construction. For example, we update the "version" as soon as it's resolved for time-travel by timestamp or load latest snapshot queries. This means in the case of the exception we include as much information as is available.

How was this patch tested?

Adds a test suite MetricsReportSuite with unit tests.

Does this PR introduce any user-facing changes?

No.

scottsand-db

Looks great! Reviewed the production code, not the tests yet

kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/SnapshotContext.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionBuilderImpl.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/SnapshotReportImpl.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java

kernel/kernel-api/src/main/java/io/delta/kernel/metrics/SnapshotMetricsResult.java

scottsand-db

Just posting what we discussed in a sync! Looks great!

Would be great to use factory methods to create SnapshotReportImpls. One for success/normal cases, one for error case. This should take in the SnapshotQueryContext.
Ensure you have the same exact method / value names between SnapshotMetrics and SnapshotMetricsResult. Also, add docs to both that explain the relationship. i.e. SnapshotMetrics creates SnapshotMetricsResult; SnapshotMetricsResult is created by SnapshotMetrics

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/MetricsReportSuite.scala

scottsand-db · 2024-12-05T23:23:42Z

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/MetricsReportSuite.scala

+class MetricsReportSuite extends AnyFunSuite with TestUtils {
+
+  ///////////////////////////
+  // SnapshotReport tests //


How many different types of reports do you anticipate we will have? If test + helper code for just SnapshotReport is ~300 LOC, I wonder if we should split this up into different files?

e.g. SnapshotMetricsReportSuite extends MetricsReportTestUtils

I think for now this is okay. I think the other report types will have fewer tests (much fewer error cases). If it ends up being a lot in a later PR I can separate the suites then.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java

scottsand-db

Looks great! Left some comments and questions. Thanks!

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableImpl.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/SnapshotMetrics.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/SnapshotQueryContext.java

scottsand-db · 2025-01-06T17:40:36Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/SnapshotQueryContext.java

+import io.delta.kernel.metrics.SnapshotReport;
+import java.util.Optional;
+
+/** Stores the context for a given Snapshot query. Used to generate a {@link SnapshotReport} */


This class is named SnapshotQueryContext ... and you say that it "stores the context" ... but can you define a bit what "context" means? You don't need to list all of the information it contains .. but at high level, what does context mean?

Stores the context for a given Snapshot query. Used to generate a {@link SnapshotReport}. Here, "context" means the state and information pertaining to when a Snapshot was constructed, for which table, and its associated metrics

Can you also talk a bit about the expected lifecycle of this class. What is it coupled to?

Updated the class docs

kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/SnapshotQueryContext.java

kernel/kernel-api/src/main/java/io/delta/kernel/metrics/SnapshotReport.java

scottsand-db

Looks great! Minor comments mainly on variable/api naming. After this LGTM

kernel/kernel-api/src/main/java/io/delta/kernel/metrics/SnapshotMetricsResult.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/SnapshotMetrics.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/SnapshotQueryContext.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java

scottsand-db · 2025-01-07T19:21:32Z

kernel/kernel-api/src/main/java/io/delta/kernel/metrics/SnapshotReport.java

+
+  @Override
+  default String operationType() {
+    return DeltaOperationReport.OperationType.SNAPSHOT.toString();


Hm. If we return String still I wonder if the enum isn't as useful? what if we returned the enum value instead?

i.e. how do we expect this operationType() method to be used?

I'm fine with whatever you decide, no need to block.

My thought is that it's useful to go to DeltaOperationType.OperationType and see all the possible operation types we might encounter. I think it makes sense to be string instead of the enum to try to keep the metric return types to standard built in types.

But maybe the answer to that is we don't need the enum and it's possible to just look at all the metrics interfaces we have added.

My thought is that it's useful to go to DeltaOperationReport.OperationType and see all the possible operation types we might encounter.

But there's nothing enforcing this, right? It's just declaring an enum -- children of DeltaOperationReport don't actually need to add a new type to this enum, they could just return an arbitrary string for String operationType().

If we had operationType return an enum, then it's fair to require and expect that children will add new values for the enum.

But maybe the answer to that is we don't need the enum and it's possible to just look at all the metrics interfaces we have added.

I think the question is: how will the result of operationType be used? is it just for printing? do we want to check for a particular value (in this case, an enum would be nice)

Hmm. I think we would probably wouldn't check for a specific value. If you want a snapshot report you would check if it's an instance of SnapshotReport.

I think i'll remove the enum and keep operationType to return a string.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/Timer.java

…ricsReporter for the default engine (#3904)  #### Which Delta project/connector is this regarding?  - [ ] Spark - [ ] Standalone - [ ] Flink - [X] Kernel - [ ] Other (fill in here) ## Description This PR is based off of #3903 See the diff for just this PR [here](https://github.com/delta-io/delta/pull/3904/files/aec95cf3dc0086c37f4c45e2b3e192b7b881768c..678ac473f4de65a8f7fd770696aad2d31a15aef7) Adds a JSON serializer for metrics reports with serialization logic for SnapshotReport. Also adds a `LoggingMetricsReporter` to the default implementation which simply logs the JSON serialized reports using Log4J. ## How was this patch tested? Adds a test suite. ## Does this PR introduce _any_ user-facing changes? No.

allisonport-db force-pushed the metrics-2 branch 2 times, most recently from e1dbe30 to ceb5a56 Compare November 27, 2024 00:36

allisonport-db changed the title ~~Metrics 2~~ [Kernel][Metrics][PR#2] Add SnapshotReport for reporting snapshot construction Nov 27, 2024

allisonport-db mentioned this pull request Nov 27, 2024

[Kernel] Support metrics and event logging #3905

Open

5 tasks

allisonport-db requested review from scottsand-db and vkorukanti November 27, 2024 01:16

allisonport-db mentioned this pull request Nov 27, 2024

[Kernel][Metrics][PR#3] Metrics report JSON serializer and LoggingMetricsReporter for the default engine #3904

Merged

5 tasks

scottsand-db requested changes Dec 4, 2024

View reviewed changes

Snapshot report and tests

e1d8846

allisonport-db force-pushed the metrics-2 branch from ceb5a56 to db5c16c Compare December 5, 2024 21:52

Respond to comments

8e1f651

allisonport-db force-pushed the metrics-2 branch from db5c16c to 8e1f651 Compare December 5, 2024 21:56

allisonport-db requested a review from scottsand-db December 5, 2024 22:09

scottsand-db requested changes Dec 5, 2024

View reviewed changes

scottsand-db reviewed Jan 3, 2025

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java Outdated Show resolved Hide resolved

allisonport-db added 4 commits January 3, 2025 18:00

Save

6724a1a

Merge remote-tracking branch 'delta-io/master' into metrics-2-merge

68f1860

Merge follow-up

eb46659

respond to comment

aec95cf

allisonport-db requested a review from scottsand-db January 4, 2025 02:57

scottsand-db requested changes Jan 6, 2025

View reviewed changes

scottsand-db assigned allisonport-db Jan 6, 2025

Respond to comments

b615ae9

allisonport-db requested a review from scottsand-db January 7, 2025 03:05

scottsand-db requested changes Jan 7, 2025

View reviewed changes

allisonport-db added 3 commits January 7, 2025 13:06

Respond to comments

83577ac

Remove operation type enum

8173386

Fix scala style

67aca55

scottsand-db reviewed Jan 8, 2025

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/Timer.java Outdated Show resolved Hide resolved

Remove unused variable

89c246e

scottsand-db approved these changes Jan 8, 2025

View reviewed changes

Remove whitespace

da7e3c2

allisonport-db merged commit 40ba346 into delta-io:master Jan 8, 2025
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kernel][Metrics][PR#2] Add SnapshotReport for reporting snapshot construction #3903

[Kernel][Metrics][PR#2] Add SnapshotReport for reporting snapshot construction #3903

allisonport-db commented Nov 26, 2024 •

edited

Loading

scottsand-db left a comment

scottsand-db left a comment

scottsand-db Dec 5, 2024

allisonport-db Jan 4, 2025

scottsand-db left a comment

scottsand-db Jan 6, 2025

scottsand-db Jan 6, 2025

allisonport-db Jan 7, 2025

scottsand-db left a comment

scottsand-db Jan 7, 2025

allisonport-db Jan 7, 2025

allisonport-db Jan 7, 2025

scottsand-db Jan 7, 2025

scottsand-db Jan 7, 2025

allisonport-db Jan 7, 2025

scottsand-db Jan 8, 2025

[Kernel][Metrics][PR#2] Add SnapshotReport for reporting snapshot construction #3903

[Kernel][Metrics][PR#2] Add SnapshotReport for reporting snapshot construction #3903

Conversation

allisonport-db commented Nov 26, 2024 • edited Loading

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

scottsand-db left a comment

Choose a reason for hiding this comment

scottsand-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottsand-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottsand-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonport-db commented Nov 26, 2024 •

edited

Loading